Conversation
…rror-indicating-that-the-sandbox-is-not-running-when-e2b-1327
jakubno
requested changes
Jan 15, 2025
Member
jakubno
left a comment
jakubno
left a comment
There was a problem hiding this comment.
the DNS is now shared for API and orchestrator. I think it will behave the same, because there's nothing running on 3003 port, but it would be better and cleaner to split it
mlejva
requested changes
Jan 15, 2025
…rror-indicating-that-the-sandbox-is-not-running-when-e2b-1327
Contributor
Author
|
@jakubno split the DNS package between api/ and orchestrator/ , with only the latter having the special "sandbox not found" routing |
![callstackai[bot]](https://avatars.githubusercontent.com/in/1040204?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Key Issues
Setting a TTL of 0 can lead to performance issues due to excessive DNS queries; consider using a positive TTL value for caching. The code lacks validation for empty DNS questions, which could result in undefined behavior. Assumptions about the format of q.Name may cause errors if the expected '-' character is missing; implement validation or safer parsing.
…r (which session proxy uses)
jakubno
approved these changes
Jan 16, 2025
mlejva
approved these changes
Jan 16, 2025
0div
deleted the
return-error-indicating-that-the-sandbox-is-not-running-when-e2b-1327
branch
0div
added a commit
to e2b-dev/E2B
that referenced
this pull request
Apr 25, 2025
Added a test in `js-sdk` for e2b-dev/infra#231 --------- Co-authored-by: Jakub Novák <jakub@e2b.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
[re-using previous work from @dobrac in #224]
To return a meaningful error during the DNS resolution in API from
client-proxywe route the traffic to a "dummy" server that serves the exception if the sandbox cannot be found.Per feedback, also split the DNS package between api/ and orchestrator/ , with only the latter having the special "sandbox not found" routing.
Test
✨
Description by Callstackai
This PR introduces a new DNS handling feature that provides a meaningful 502 error message when a sandbox is not running or does not exist. It also refactors the DNS package structure and updates the configuration for the client proxy.
Diagrams of code changes
sequenceDiagram participant Client participant DNS Server participant Records Map Note over DNS Server: Initialized with empty records map alt Add Sandbox Record Client->>DNS Server: Add(sandboxID, ip) DNS Server->>Records Map: Insert(hostname, ip) end alt DNS Request Client->>DNS Server: DNS Query DNS Server->>Records Map: Get(sandboxID) alt Record Found Records Map-->>DNS Server: Return IP DNS Server-->>Client: Response with Sandbox IP else Record Not Found Records Map-->>DNS Server: Not Found DNS Server-->>Client: Response with 127.0.0.1 end end alt Remove Sandbox Record Client->>DNS Server: Remove(sandboxID, ip) DNS Server->>Records Map: Remove Entry endFiles Changed
This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.conf. See list of supported languages.